Refactor/peer persistence #327
Conversation
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
📝 WalkthroughWalkthroughPeer persistence was migrated from a file-backed Changes
Sequence Diagram(s)sequenceDiagram
participant Manager as PeerNetworkManager
participant Storage as PersistentPeerStorage
participant FS as Filesystem
Note over Manager,Storage: Saving reputations
Manager->>Storage: save_peers_reputation(reputations)
Storage->>FS: atomic_write(reputations.json)
FS-->>Storage: write_ok
Storage-->>Manager: ok
Note over Manager,Storage: Loading peers on startup
Manager->>Storage: load_peers()
Storage->>FS: read(peers.dat)
FS-->>Storage: data
Storage-->>Manager: Vec<SocketAddr>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
changed the base branch from
v0.41-dev
to
refactor/storage-manager-trait
ZocoLini
force-pushed
the
refactor/peer-persistence
branch
from
5d78f43 to
2d4550e
Compare
github-actions
bot
removed
the
merge-conflict
ZocoLini
force-pushed
the
refactor/storage-manager-trait
branch
from
2bc0490 to
e40b953
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
ZocoLini
force-pushed
the
refactor/storage-manager-trait
branch
from
e40b953 to
0248257
Compare
ZocoLini
force-pushed
the
refactor/peer-persistence
branch
from
3ca989d to
4a7925a
Compare
github-actions
bot
removed
the
merge-conflict
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dash-spv/src/storage/mod.rs (1)
209-250: Missing peers re-initialization inclear()method.After clearing storage and re-opening all storage components (lines 233-244), the
peersstorage is not re-initialized. This will leaveself.peerspointing to the old storage instance with stale data, potentially causing issues after aclear()operation.🐛 Add peers re-initialization in clear()
self.masternodestate = Arc::new(RwLock::new(PersistentMasternodeStateStorage::open(storage_path).await?)); + self.peers = Arc::new(RwLock::new(PersistentPeerStorage::open(storage_path).await?)); // Restart the background worker for future operations self.start_worker().await;
🤖 Fix all issues with AI agents
In @dash-spv/src/storage/peers.rs:
- Around line 79-94: Remove the blocking std::fs::create_dir_all call and its
unwrap; instead compute the parent once using peers_file.parent().ok_or(...) (as
in peers_file_parent) and call the async
tokio::fs::create_dir_all(peers_file_parent).await to create directories;
eliminate the duplicate directory creation block and ensure any errors are
mapped to StorageError::WriteFailed where appropriate (keep the existing buffer
encoding loop and error handling for item.consensus_encode).
- Around line 101-127: In load_peers, replace blocking std::fs operations with
async equivalents: use tokio::fs::try_exists(peers_file) to check existence,
open the file with tokio::fs::File::open and wrap it in tokio::io::BufReader, or
simpler use tokio::fs::read(peers_file) to load the whole file into a Vec<u8>
and decode from a std::io::Cursor; then feed that reader/bytes to
AddrV2Message::consensus_decode (or its slice-based variant) instead of using
the blocking BufReader/File and std::fs::exists; ensure imports include
tokio::fs::File, tokio::fs::try_exists, tokio::fs::read and tokio::io::BufReader
and update error handling around consensus_decode accordingly.
- Around line 148-159: In load_peers_reputation, replace the blocking
reputation_file.exists() call with the async
tokio::fs::try_exists(reputation_file).await and handle its Result (e.g., let
exists = tokio::fs::try_exists(reputation_file).await?; if !exists { return
Ok(HashMap::new()); }) so the function stays fully async and errors propagate
consistently alongside the existing tokio::fs::read_to_string(...).await?.
🧹 Nitpick comments (3)
dash-spv/tests/wallet_integration_test.rs (1)
18-31: Inconsistent storage paths between network and disk storage managers.The test creates two separate temporary directories:
- Line 18:
temp_dirused forClientConfig.with_storage_path()→ used byPeerNetworkManagerfor peer storage- Line 29: A new
TempDirforDiskStorageManagerThis means peer data and other storage (headers, filters, etc.) are written to different directories. While this works for basic testing, it creates an inconsistency that could mask integration issues where peers and storage should share the same root path.
Consider using a single temporary directory for both:
♻️ Suggested fix to use consistent storage paths
async fn create_test_client( ) -> DashSpvClient<WalletManager<ManagedWalletInfo>, PeerNetworkManager, DiskStorageManager> { let temp_dir = tempfile::TempDir::new().expect("Failed to create temporary directory"); + let storage_path = temp_dir.path().to_path_buf(); + let config = ClientConfig::testnet() .without_filters() .without_masternodes() - .with_storage_path(temp_dir.path().to_path_buf()); + .with_storage_path(storage_path.clone()); // Create network manager let network_manager = PeerNetworkManager::new(&config).await.unwrap(); // Create storage manager - let storage_manager = - DiskStorageManager::new(TempDir::new().expect("Failed to create tmp dir").path()) - .await - .expect("Failed to create tmp storage"); + let storage_manager = DiskStorageManager::new(&storage_path) + .await + .expect("Failed to create tmp storage"); + + // Keep temp_dir alive by leaking it (or store it in the client if possible) + std::mem::forget(temp_dir);Note: If you keep separate directories intentionally for test isolation, consider adding a comment explaining this design choice.
dash-spv/tests/header_sync_test.rs (1)
264-280: Same storage path inconsistency as wallet_integration_test.rs.Similar to the wallet integration test, this creates two separate temporary directories - one for
ClientConfig(used byPeerNetworkManager) and another forDiskStorageManager. This means peer storage and block header storage will be in different locations.For consistency with production behavior where all storage shares a common root, consider using the same path for both managers.
dash-spv/src/storage/peers.rs (1)
162-206: Consider adding tests for reputation persistence methods.The tests cover peer save/load well, but
save_peers_reputationandload_peers_reputationare not tested. Adding tests for these methods would improve coverage and confidence in the serialization/deserialization logic.💡 Example test structure
#[tokio::test] async fn test_persistent_peer_storage_reputation_save_load() { let temp_dir = TempDir::new().expect("Failed to create temporary directory"); let store = PersistentPeerStorage::open(temp_dir.path()) .await .expect("Failed to open persistent peer storage"); let mut reputations = HashMap::new(); let addr: SocketAddr = "192.168.1.1:9999".parse().unwrap(); let reputation = PeerReputation::default(); // or construct with specific values reputations.insert(addr, reputation); store.save_peers_reputation(&reputations) .await .expect("Failed to save reputations"); let loaded = store.load_peers_reputation() .await .expect("Failed to load reputations"); assert_eq!(loaded.len(), 1); assert!(loaded.contains_key(&addr)); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignoredash-spv/src/network/manager.rsdash-spv/src/network/mod.rsdash-spv/src/network/persist.rsdash-spv/src/network/reputation.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/peers.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/tests/wallet_integration_test.rs
💤 Files with no reviewable changes (3)
- dash-spv/src/network/persist.rs
- dash-spv/src/network/mod.rs
- .gitignore
🧰 Additional context used
📓 Path-based instructions (8)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/manager.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/peers.rs
dash-spv/tests/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/tests/**/*.rs: Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/manager.rsdash-spv/src/network/reputation.rsdash-spv/src/storage/peers.rs
**/tests/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/tests/**/*.rs: Use descriptive test names (e.g.,test_parse_address_mainnet)
Mark network-dependent or long-running tests with#[ignore]and run with-- --ignored
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rs
**/*integration*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Write integration tests for network operations
Files:
dash-spv/tests/wallet_integration_test.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/network/reputation_tests.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/peers.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/network/**/*.rs: Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Implement configurable timeouts with recovery mechanisms for network operations
Files:
dash-spv/src/network/reputation_tests.rsdash-spv/src/network/manager.rsdash-spv/src/network/reputation.rs
🧠 Learnings (27)
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/client/**/*.rs : Use the DashSpvClient high-level API with proper configuration via ClientConfig for client initialization
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
dash-spv/tests/wallet_integration_test.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-06-26T16:01:37.609Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:01:37.609Z
Learning: The mempool tracking infrastructure (UnconfirmedTransaction, MempoolState, configuration, and mempool_filter.rs) is fully implemented and integrated in the Dash SPV client as of this PR, including client logic, FFI APIs, and tests.
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/src/storage/mod.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`
Applied to files:
dash-spv/tests/wallet_integration_test.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/manager.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/src/network/manager.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/src/storage/mod.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/tests/handshake_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/storage/mod.rsdash-spv/src/network/manager.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations
Applied to files:
dash-spv/tests/header_sync_test.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Follow a layered, trait-based architecture with clear separation of concerns across modules: client/, network/, storage/, sync/, validation/, wallet/, with trait-based abstractions for swappable implementations
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/*.rs : Use `BTreeMap` for ordered data (accounts, transactions) and `HashMap` for lookups (address mappings); apply memory management strategies for old transaction data
Applied to files:
dash-spv/src/storage/mod.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/storage/mod.rsdash-spv/src/network/manager.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Unit tests should live alongside code with `#[cfg(test)]` annotation; integration tests use the `tests/` directory
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Mark network-dependent or long-running tests with `#[ignore]` and run with `-- --ignored`
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Applied to files:
dash-spv/src/network/reputation_tests.rs
🧬 Code graph analysis (6)
dash-spv/tests/handshake_test.rs (2)
dash-spv/src/network/manager.rs (1)
new(78-117)dash-spv/src/client/config.rs (1)
new(210-217)
dash-spv/tests/header_sync_test.rs (2)
dash-spv/src/network/manager.rs (1)
new(78-117)dash-spv/src/storage/mod.rs (1)
new(99-143)
dash-spv/src/storage/mod.rs (2)
dash-spv/src/network/manager.rs (2)
new(78-117)clone(1036-1059)dash-spv/src/storage/peers.rs (1)
open(57-63)
dash-spv/src/network/reputation_tests.rs (3)
dash-spv/src/network/manager.rs (1)
new(78-117)dash-spv/src/storage/mod.rs (2)
new(99-143)open(52-52)dash-spv/src/storage/peers.rs (1)
open(57-63)
dash-spv/src/network/manager.rs (2)
dash-spv/src/storage/peers.rs (1)
open(57-63)dash-spv/src/storage/mod.rs (2)
open(52-52)new(99-143)
dash-spv/src/storage/peers.rs (3)
dash-spv/src/storage/io.rs (1)
atomic_write(24-57)dash-spv/src/storage/mod.rs (4)
open(52-52)persist(54-54)persist(193-204)new(99-143)dash-spv/src/network/manager.rs (1)
new(78-117)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Windows / tools
- GitHub Check: Ubuntu / ffi
- GitHub Check: Ubuntu ARM / tools
- GitHub Check: macOS / wallet
- GitHub Check: macOS / ffi
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: MSRV Check (1.89)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Thread Sanitizer
- GitHub Check: Address Sanitizer
- GitHub Check: conflict-check
🔇 Additional comments (17)
dash-spv/tests/handshake_test.rs (1)
76-77: LGTM! Proper test isolation with temporary storage.The test now correctly uses a temporary directory for storage, ensuring test isolation and alignment with the new
PersistentPeerStoragebackend. Thetemp_dirlifetime is appropriate as it persists for the test duration.dash-spv/src/network/reputation_tests.rs (2)
5-6: LGTM! Import aligns with new storage architecture.The import correctly brings in
PersistentStoragefrom the storage module, supporting the storage-backed persistence pattern.
66-73: LGTM! Test correctly migrated to storage-backed persistence.The test properly:
- Creates a temporary directory for isolation
- Opens
PersistentPeerStoragewith the temp path- Uses storage handles for
save_to_storageandload_from_storageThis aligns with the PR's goal of moving to storage-backed persistence.
dash-spv/src/storage/mod.rs (3)
8-12: LGTM! Module declarations for new storage components.The new
ioandpeersmodules are correctly declared in the module tree.
43-43: LGTM! Public exports for peer storage types.The
PeerStoragetrait andPersistentPeerStorageimplementation are correctly exported, making them available to the network module.
90-90: LGTM! Peers storage properly integrated into DiskStorageManager.The
peersfield is:
- Correctly typed as
Arc<RwLock<PersistentPeerStorage>>- Properly initialized in
new()- Cloned for the background worker
- Persisted in both the periodic worker loop and explicit
persist()callsThis follows the same pattern as other storage components.
Also applies to: 133-133, 162-162, 179-179, 203-203
dash-spv/src/network/manager.rs (3)
30-30: LGTM! Updated imports for storage-backed persistence.The imports correctly bring in
PeerStoragetrait,PersistentPeerStorageimplementation, andPersistentStoragetrait for the new storage architecture.
42-42: LGTM! PeerNetworkManager migrated to PersistentPeerStorage.The peer store field and initialization are correctly updated:
- Field type changed from
PeerStoretoPersistentPeerStorage- Uses
PersistentStorage::open()trait method for initialization- Reputation data is loaded on startup with graceful error handling
Also applies to: 84-90
741-743: LGTM! Reputation persistence uses new storage interface.The
save_to_storagecalls in both the maintenance loop and shutdown path correctly pass thepeer_storereference, aligning with the new&PersistentPeerStoragesignature.Also applies to: 1015-1017
dash-spv/src/network/reputation.rs (6)
8-8: LGTM! Updated imports for serde and storage.Correctly imports
Deserializerfor custom deserialization and the new storage types from the storage module.Also applies to: 15-15
80-92: LGTM! New constants and helper for reputation bounds.The new constants properly define:
MAX_MISBEHAVIOR_SCORE(100) - ban thresholdMIN_MISBEHAVIOR_SCORE(-50) - best possible reputationMAX_BAN_COUNTandMAX_ACTION_COUNT- guards against corrupted dataThe
default_instant()helper provides a consistent default forInstantfields.
94-134: LGTM! Defensive deserialization with clamping.The custom deserializers provide robust handling of potentially corrupted or malicious persistence data:
clamp_peer_score: Clamps to valid score range with loggingclamp_peer_ban_count: Caps excessive ban countsclamp_peer_connection_attempts: Limits connection attemptsThis is good defensive programming for data loaded from disk.
137-171: LGTM! PeerReputation serde configuration.The struct correctly:
- Derives
SerializeandDeserialize- Uses custom deserializers for clamped fields
- Skips
Instant-based fields that can't be serialized- Provides defaults for skipped fields via
default_instant()
219-219: LGTM! Score clamping uses consistent bounds.Both
apply_decayandupdate_reputationnow use the named constantsMIN_MISBEHAVIOR_SCOREandMAX_MISBEHAVIOR_SCOREfor consistent bounds enforcement.Also applies to: 283-283
454-458: LGTM! Storage-backed persistence implementation.The persistence methods correctly:
- Accept
&PersistentPeerStorageinstead of file paths- Delegate to the storage trait methods
- Include validation during load (connection count consistency, action count sanity checks)
One note on lines 483-485: Previously banned peers start with
score.max(50), which means they're closer to the ban threshold. This is intentional defensive behavior to be more cautious with historically problematic peers.Also applies to: 461-497
dash-spv/src/storage/peers.rs (2)
22-53: LGTM! Clean trait and struct design.The trait design with immutable
&selfreferences aligns well with concurrent access patterns, and the helper methods provide clear path construction logic.
55-69: LGTM! Appropriate PersistentStorage implementation.The no-op
persist()is correctly documented as data is written immediately during save operations.
ZocoLini
force-pushed
the
refactor/peer-persistence
branch
from
732c3ac to
68750bc
Compare
github-actions
bot
added
the
merge-conflict
|
This PR has merge conflicts with the base branch. Please rebase or merge the base branch into your branch to resolve them. |
github-actions
bot
removed
the
merge-conflict
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dash-spv/src/storage/peers.rs (2)
149-160: Consider using spawn_blocking for consistency with load_peers.While reputation JSON files are typically small, using
spawn_blockingwould maintain consistency with theload_peersimplementation pattern and prevent potential executor blocking for larger reputation datasets.♻️ Proposed refactor for consistency
async fn load_peers_reputation(&self) -> StorageResult<HashMap<SocketAddr, PeerReputation>> { let reputation_file = self.peers_reputation_file(); if !fs::try_exists(&reputation_file).await? { return Ok(HashMap::new()); } - let json = fs::read_to_string(reputation_file).await?; - serde_json::from_str(&json).map_err(|e| { - StorageError::ReadFailed(format!("Failed to deserialize peers reputations: {e}")) - }) + tokio::task::spawn_blocking(move || { + let json = std::fs::read_to_string(reputation_file)?; + serde_json::from_str(&json).map_err(|e| { + StorageError::ReadFailed(format!("Failed to deserialize peers reputations: {e}")) + }) + }) + .await + .map_err(|e| StorageError::ReadFailed(format!("Failed to load peers reputation: {e}")))? }
163-207: Add test coverage for reputation persistence.The tests cover basic peer save/load operations well, but there's no coverage for
save_peers_reputationandload_peers_reputation. Consider adding tests for:
- Saving and loading peer reputations
- Loading from empty reputation storage
- Multiple peers and reputations
📝 Example test for reputation persistence
#[tokio::test] async fn test_persistent_peer_storage_reputation() { let temp_dir = TempDir::new().expect("Failed to create temporary directory for test"); let store = PersistentPeerStorage::open(temp_dir.path()) .await .expect("Failed to open persistent peer storage"); let addr: SocketAddr = "192.168.1.1:9999".parse().unwrap(); let mut reputations = HashMap::new(); reputations.insert(addr, PeerReputation::default()); // Adjust based on PeerReputation API store.save_peers_reputation(&reputations) .await .expect("Failed to save reputations"); let loaded = store.load_peers_reputation() .await .expect("Failed to load reputations"); assert_eq!(loaded.len(), 1); assert!(loaded.contains_key(&addr)); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dash-spv/src/storage/peers.rsdash-spv/tests/header_sync_test.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- dash-spv/tests/header_sync_test.rs
🧰 Additional context used
📓 Path-based instructions (3)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/storage/peers.rs
dash-spv/src/storage/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Files:
dash-spv/src/storage/peers.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/storage/peers.rs
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/src/**/*.rs : Use thread-local storage for error propagation via `dash_spv_ffi_get_last_error()` function
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Prefer `async`/`await` via `tokio` for asynchronous operations
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Tokio channels for inter-component message passing between async tasks
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Implement configurable timeouts with recovery mechanisms for network operations
Applied to files:
dash-spv/src/storage/peers.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Use tokio runtime for async operations in Rust
Applied to files:
dash-spv/src/storage/peers.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Windows / tools
- GitHub Check: Windows / ffi
- GitHub Check: macOS / ffi
- GitHub Check: macOS / spv
- GitHub Check: macOS / tools
- GitHub Check: fuzz (hashes_sha512_256)
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_sha512)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: Address Sanitizer
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Thread Sanitizer
- GitHub Check: conflict-check
🔇 Additional comments (7)
dash-spv/src/storage/peers.rs (7)
1-16: LGTM: Clean import organization.The imports are well-organized and all necessary for the functionality. The mix of sync (std::fs::File) and async (tokio::fs) types is appropriate for the spawn_blocking pattern used later.
18-33: LGTM: Well-designed trait interface.The
PeerStoragetrait provides a clean separation between peer address storage and reputation tracking, with appropriate async signatures consistent with the storage layer patterns.
35-49: LGTM: Clean helper methods for path management.The helper methods provide clear, centralized path construction for peer data and reputation files.
51-65: LGTM: Appropriate implementation of PersistentStorage.The no-op
persist()method is well-documented and makes sense since data is atomically written during save operations.
69-91: LGTM: Robust peer persistence with atomic writes.The implementation correctly uses consensus encoding and atomic writes to safely persist peer data. The defensive parent directory check is good practice even though the path structure guarantees its existence.
93-128: Excellent use of spawn_blocking for sync I/O operations.The implementation correctly offloads synchronous file I/O operations to a blocking thread pool using
spawn_blocking, preventing the tokio executor from being blocked. The EOF handling is proper, and the silent filtering of invalid socket addresses (line 125) provides resilience against corrupted entries.
130-147: LGTM: Consistent reputation persistence pattern.The implementation follows the same safe persistence pattern as
save_peers, with appropriate JSON serialization and atomic writes. Pretty-printing provides debugging benefits with minimal overhead.
ZocoLini
force-pushed
the
refactor/peer-persistence
branch
from
d45fe90 to
5626344
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
dash-spv/src/network/manager.rs (1)
42-42: Consider using trait object for storage polymorphism.Per the coding guidelines,
Arc<dyn TraitName>should be used for trait objects to enable runtime polymorphism. UsingArc<dyn PeerStorage>instead ofArc<PersistentPeerStorage>would allow swapping storage implementations (e.g., for testing with in-memory storage).This isn't blocking since the current concrete type works correctly, but could improve testability and flexibility.
dash-spv/src/network/reputation.rs (1)
156-166: Consider adding clamp deserializers forpositive_actions,negative_actions, andsuccessful_connections.The
connection_attemptsfield has a clamp deserializer (clamp_peer_connection_attempts), butpositive_actions,negative_actions, andsuccessful_connectionsdon't. The validation inload_from_storage(lines 474-480) checks these againstMAX_ACTION_COUNTbut rejects the entry entirely rather than clamping.For consistency, consider either:
- Adding clamp deserializers for these fields (like
connection_attempts)- Or documenting why these fields should reject rather than clamp
This is a minor inconsistency that doesn't affect correctness.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dash-spv/src/network/manager.rsdash-spv/src/network/reputation.rsdash-spv/src/network/reputation_tests.rs
🧰 Additional context used
📓 Path-based instructions (4)
dash-spv/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/**/*.rs: Use async/await throughout the codebase, built on tokio runtime
Use Arc for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Use Tokio channels for inter-component message passing between async tasks
Maintain minimum Rust version (MSRV) of 1.89 and use only compatible syntax and features
Files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/reputation.rs
dash-spv/src/network/**/*.rs
📄 CodeRabbit inference engine (dash-spv/CLAUDE.md)
dash-spv/src/network/**/*.rs: Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Implement configurable timeouts with recovery mechanisms for network operations
Files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/reputation.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: MSRV (Minimum Supported Rust Version) is 1.89; ensure compatibility with this version for all builds
Unit tests should live alongside code with#[cfg(test)]annotation; integration tests use thetests/directory
Usesnake_casefor function and variable names
UseUpperCamelCasefor types and traits
UseSCREAMING_SNAKE_CASEfor constants
Format code withrustfmtbefore commits; ensurecargo fmt --allis run
Runcargo clippy --workspace --all-targets -- -D warningsfor linting; avoid warnings in CI
Preferasync/awaitviatokiofor asynchronous operations
**/*.rs: Never hardcode network parameters, addresses, or keys in Rust code
Use proper error types (thiserror) and propagate errors appropriately in Rust
Use tokio runtime for async operations in Rust
Use conditional compilation with feature flags for optional features
Write unit tests for new functionality in Rust
Format code using cargo fmt
Run clippy with all features and all targets, treating warnings as errors
Never log or expose private keys in any code
Always validate inputs from untrusted sources in Rust
Use secure random number generation for keys
Files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/reputation.rs
**/*test*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*test*.rs: Test both mainnet and testnet configurations
Use proptest for property-based testing where appropriate
Files:
dash-spv/src/network/reputation_tests.rs
🧠 Learnings (20)
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/network/**/*.rs : Use DNS-first peer discovery with automatic DNS seeds (dnsseed.dash.org, testnet-seed.dashdot.io) when no explicit peers are configured; implement immediate startup with 10-second delay only for subsequent peer searches
Applied to files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/reputation.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use Arc<dyn TraitName> for trait objects to enable runtime polymorphism for NetworkManager and StorageManager
Applied to files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-06-26T15:54:02.509Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T15:54:02.509Z
Learning: The `StorageManager` trait in `dash-spv/src/storage/mod.rs` uses `&mut self` methods but is also `Send + Sync`, and implementations often use interior mutability for concurrency. This can be confusing, so explicit documentation should clarify thread-safety expectations and the rationale for the API design.
Applied to files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rsdash-spv/src/network/reputation.rs
📚 Learning: 2025-06-26T16:02:42.390Z
Learnt from: DCG-Claude
Repo: dashpay/rust-dashcore PR: 0
File: :0-0
Timestamp: 2025-06-26T16:02:42.390Z
Learning: Passing exclusive mutable references to network and storage managers into the sync manager in async Rust can lead to borrow checker issues or runtime contention if concurrent access is needed elsewhere. It's advisable to document this architectural tradeoff and consider refactoring to interior mutability or message passing for shared access as the codebase evolves.
Applied to files:
dash-spv/src/network/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/sync/**/*.rs : Implement sequential phase-based synchronization via SyncManager with phases progressing in order: Headers → Masternode List → Filter Headers → Filters → Blocks
Applied to files:
dash-spv/src/network/manager.rsdash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/storage/**/*.rs : Store headers in 10,000-header segments with index files in headers/ directory, filter headers and compact block filters in filters/ directory, and state in state/ directory
Applied to files:
dash-spv/src/network/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/src/error.rs : Use domain-specific error types (NetworkError, StorageError, SyncError, ValidationError, SpvError) rather than generic error handling
Applied to files:
dash-spv/src/network/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/**/*.rs : Use async/await throughout the codebase, built on tokio runtime
Applied to files:
dash-spv/src/network/manager.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Organize tests into unit tests (in-module), integration tests (tests/ directory), real network tests (with live Dash Core nodes), and performance benchmarks
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Cover critical parsing, networking, SPV, and wallet flows in tests; add regression tests for fixes; consider property tests with `proptest`
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-16T09:03:55.811Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv/CLAUDE.md:0-0
Timestamp: 2025-12-16T09:03:55.811Z
Learning: Applies to dash-spv/tests/**/*.rs : Integration tests should gracefully handle unavailable Dash Core nodes at 127.0.0.1:9999 without failing the test suite
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-01T07:59:58.608Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: dash-spv-ffi/CLAUDE.md:0-0
Timestamp: 2025-12-01T07:59:58.608Z
Learning: Applies to dash-spv-ffi/tests/unit/**/*.rs : Add corresponding unit tests in `tests/unit/` for each new FFI function
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Organize unit tests by functionality: separate test files for BIP32, mnemonics, addresses, derivation paths, and PSBT operations
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-19T00:07:22.904Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: key-wallet/CLAUDE.md:0-0
Timestamp: 2025-12-19T00:07:22.904Z
Learning: Applies to key-wallet/**/tests/**/*.rs : Use deterministic testing with known test vectors and fixed seeds for reproducible results
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*integration*.rs : Write integration tests for network operations
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/*.rs : Unit tests should live alongside code with `#[cfg(test)]` annotation; integration tests use the `tests/` directory
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Use descriptive test names (e.g., `test_parse_address_mainnet`)
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*test*.rs : Test both mainnet and testnet configurations
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-22T17:59:51.097Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-22T17:59:51.097Z
Learning: Applies to **/tests/**/*.rs : Mark network-dependent or long-running tests with `#[ignore]` and run with `-- --ignored`
Applied to files:
dash-spv/src/network/reputation_tests.rs
📚 Learning: 2025-12-30T22:00:57.000Z
Learnt from: CR
Repo: dashpay/rust-dashcore PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T22:00:57.000Z
Learning: Applies to **/*.rs : Write unit tests for new functionality in Rust
Applied to files:
dash-spv/src/network/reputation_tests.rs
🧬 Code graph analysis (2)
dash-spv/src/network/manager.rs (2)
dash-spv/src/storage/mod.rs (1)
open(52-52)dash-spv/src/storage/peers.rs (1)
open(53-59)
dash-spv/src/network/reputation_tests.rs (2)
dash-spv/src/storage/mod.rs (1)
open(52-52)dash-spv/src/storage/peers.rs (1)
open(53-59)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Ubuntu ARM / rpc
- GitHub Check: macOS / spv
- GitHub Check: fuzz (hashes_json)
- GitHub Check: fuzz (hashes_cbor)
- GitHub Check: fuzz (hashes_ripemd160)
- GitHub Check: fuzz (hashes_sha1)
- GitHub Check: fuzz (dash_outpoint_string)
- GitHub Check: fuzz (dash_deserialize_block)
- GitHub Check: fuzz (dash_deserialize_witness)
- GitHub Check: fuzz (dash_deserialize_script)
- GitHub Check: fuzz (hashes_sha256)
- GitHub Check: fuzz (dash_deser_net_msg)
- GitHub Check: fuzz (dash_deserialize_address)
- GitHub Check: fuzz (dash_script_bytes_to_asm_fmt)
- GitHub Check: Pre-commit (ubuntu-latest)
- GitHub Check: Pre-commit (windows-latest)
- GitHub Check: Pre-commit (macos-latest)
- GitHub Check: Thread Sanitizer
- GitHub Check: Address Sanitizer
- GitHub Check: conflict-check
🔇 Additional comments (13)
dash-spv/src/network/reputation_tests.rs (2)
5-6: LGTM!The imports are correctly updated to use the new storage abstractions (
PersistentPeerStorageandPersistentStorage) consistent with the storage refactor across the codebase.
66-73: LGTM!The persistence test correctly migrates to the new storage abstraction pattern. Using
tempfile::TempDirensures proper test isolation, and thePersistentPeerStorage::open()followed bysave_to_storage/load_from_storagecorrectly exercises the new storage API.dash-spv/src/network/manager.rs (4)
30-30: LGTM!Import correctly updated to use the new storage module types (
PeerStorage,PersistentPeerStorage,PersistentStorage), aligning with the trait-based storage abstraction introduced in this PR.
84-90: LGTM!The initialization correctly:
- Opens storage asynchronously with proper error propagation via
?- Creates the reputation manager
- Attempts to load reputation data with graceful degradation (warning on failure)
This pattern ensures the node can start even if reputation data is missing or corrupted.
741-743: LGTM!The
&*peer_storepattern correctly dereferences theArcto obtain a reference to the underlyingPersistentPeerStoragefor the trait method call. Error handling appropriately logs warnings without failing the maintenance loop.
1015-1017: LGTM!Shutdown correctly saves reputation data to ensure persistence across restarts. The error handling appropriately logs warnings without blocking the shutdown process.
dash-spv/src/network/reputation.rs (7)
8-8: LGTM!The import updates correctly bring in the required serde traits (
Deserialize,Deserializer,Serialize) and thePeerStoragetrait for the storage abstraction.Also applies to: 15-15
80-92: LGTM!The new constants establish sensible bounds:
MAX_MISBEHAVIOR_SCORE(100) aligns with the ban thresholdMIN_MISBEHAVIOR_SCORE(-50) allows meaningful positive reputationMAX_BAN_COUNTandMAX_ACTION_COUNTprevent potential overflow from corrupted dataThe
default_instant()function provides a reasonable default for#[serde(skip)]fields.
94-134: LGTM!The custom deserializers implement defensive validation that protects against corrupted or malicious data. The warning logs help identify when clamping occurs, aiding debugging. This approach moves validation into serde's deserialization pipeline, which is cleaner than post-deserialization validation.
219-219: LGTM!The decay logic correctly clamps the score to
MIN_MISBEHAVIOR_SCORE(-50), preventing reputation from decaying below the best possible score. This aligns with the new bounds system.
282-283: LGTM!The score update correctly uses
.clamp(MIN_MISBEHAVIOR_SCORE, MAX_MISBEHAVIOR_SCORE)to ensure the score stays within valid bounds after any reputation change.
454-458: LGTM!The
save_to_storagemethod correctly:
- Uses generic
&impl PeerStoragefor flexibility- Delegates to the storage abstraction
- Maps storage errors to
std::io::Errorfor API compatibility
461-497: LGTM!The
load_from_storagemethod implements robust validation:
- Clamps
successful_connectionsto not exceedconnection_attempts- Skips entries with corrupted action counts (defensive rejection)
- Previously banned peers start with score ≥50 (halfway to ban threshold), appropriately distrusting them
The logging provides good visibility into loaded/skipped entries.
xdustinface
left a comment
xdustinface
left a comment
There was a problem hiding this comment.
What about just creating a struct like below to merge it both into one so we don't have two separate files / APIs and only one manager for all the peer stuff in the network manager?
struct StoredPeer {
address: AddrV2Message,
reputation: PeerReputation,
}If not for this PR, maybe as follow up?
|
|
||
| const MAX_ACTION_COUNT: u64 = 1_000_000; | ||
|
|
||
| fn default_instant() -> Instant { |
There was a problem hiding this comment.
Do we really need an extra helper for this?
| pub banned_until: Option<Instant>, | ||
|
|
||
| /// Last time the reputation was updated | ||
| #[serde(skip, default = "default_instant")] |
There was a problem hiding this comment.
| #[serde(skip, default = "default_instant")] | |
| #[serde(skip, default = "Instant::now")] |
Why not just like this ^ ?
The peers and peers reputation storage logic moved into the storage module following the trait pattern introduced by PR #311. Also moved PeerReputation values validation logic into serde's pipeline
This PR is built on top of #311
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.